Conversation
- Implement CsrfAdapter for H3 events with server middleware - Add useCsrfToken and useCsrfFetch Vue composables (SSR-safe with useState) - Add client plugin for CSRF token initialization - Re-export core types and crypto utilities from module - Add example Nuxt app with strategy selector demo - Add .editorconfig
WalkthroughBehold: a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Middleware as Nuxt Middleware
participant Adapter as NuxtAdapter (H3)
participant Protector as CSRF Protector
participant Response as H3 Response
Client->>Middleware: HTTP request (headers, cookies, body)
Middleware->>Adapter: extractRequest(event)
Adapter->>Adapter: parse headers, cookies, body (cache body)
Adapter-->>Middleware: CsrfRequest
Middleware->>Protector: protect(CsrfRequest)
Protector->>Protector: validate strategy & token (header/cookie/body)
alt token valid
Protector-->>Middleware: { success: true, csrfResponse? }
Middleware->>Adapter: applyResponse(event, csrfResponse)
Adapter->>Response: set headers and Set-Cookie
Response-->>Client: 200 OK (+ token/header if issued)
else token invalid
Protector-->>Middleware: { success: false, reason }
Middleware-->>Client: 403 Forbidden (reason)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
.editorconfig (1)
14-16: A trifle incomplete — consider shielding lock files more fully.The section wisely unsets
indent_sizeandinsert_final_newlineto ward off needless diffs upon the machine-forged lock files. Yettrim_trailing_whitespaceis left to inherittruefrom the global realm. While pnpm-lock.yaml shall not wander into trailing whitespace of its own accord, unsetting it fully would guard against any meddlesome editor tool that might alter what it ought not touch.🔮 Proposed refinement for the lock file section
[{pnpm-lock.yaml,*.lock}] indent_size = unset insert_final_newline = unset +trim_trailing_whitespace = unset🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.editorconfig around lines 14 - 16, The .editorconfig section for [{pnpm-lock.yaml,*.lock}] currently unsets indent_size and insert_final_newline but leaves trim_trailing_whitespace inherited (true); update that section by adding trim_trailing_whitespace = unset so the lock-file pattern in .editorconfig (the [{pnpm-lock.yaml,*.lock}] section) explicitly disables trimming trailing whitespace.packages/nuxt/vitest.config.ts (1)
4-21: Eliminate the dual source of truth betweentsconfigRawandtsconfig.test.json.The
compilerOptionsblock insidetsconfigRawon Lines 7–15 is a verbatim copy of what already lives intsconfig.test.json. Importing that file directly removes the duplication — andJSON.stringifyis also unnecessary sincetsconfigRawaccepts a plain object.♻️ Suggested simplification
import { fileURLToPath } from 'node:url' import { defineConfig } from 'vitest/config' +import tsconfigRaw from './tsconfig.test.json' export default defineConfig({ esbuild: { - tsconfigRaw: JSON.stringify({ - compilerOptions: { - target: 'ES2020', - module: 'ESNext', - moduleResolution: 'bundler', - strict: true, - esModuleInterop: true, - skipLibCheck: true, - allowSyntheticDefaultImports: true, - }, - }), + tsconfigRaw, }, test: { globals: false, root: fileURLToPath(new URL('.', import.meta.url)), }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/vitest.config.ts` around lines 4 - 21, The tsconfigRaw object in defineConfig duplicates compilerOptions already present in tsconfig.test.json; replace the inline JSON.stringify block with an imported object reference by reading/parsing tsconfig.test.json and assigning its compilerOptions (or the whole config) directly to tsconfigRaw (no JSON.stringify needed). Locate the defineConfig call and the tsconfigRaw property in vitest.config.ts, import or require the tsconfig.test.json, and set tsconfigRaw: importedConfig (or tsconfig.test.json.compilerOptions) to remove the duplicate source of truth.packages/nuxt/tsconfig.test.json (1)
1-13: Consider extending the base tsconfig to prevent strict-settings drift in tests.This config stands alone without extending the root or
packages/nuxt/tsconfig.json. In consequence, several strict flags present in the root (noUncheckedIndexedAccess,exactOptionalPropertyTypes,noImplicitOverride,noUnusedLocals, etc.) are absent here. Test code will not be checked against the same rigour as production source, which can mask real bugs.♻️ Suggested approach
{ + "extends": "../../tsconfig.json", "compilerOptions": { "target": "ES2020", "module": "ESNext", "moduleResolution": "bundler", "strict": true, "esModuleInterop": true, "skipLibCheck": true, - "allowSyntheticDefaultImports": true + "allowSyntheticDefaultImports": true, + "noEmit": true }, "include": ["src", "test"], - "exclude": ["node_modules", "dist"] + "exclude": ["node_modules", "dist"], + "references": [{ "path": "../core" }] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/tsconfig.test.json` around lines 1 - 13, The test tsconfig (tsconfig.test.json) is missing an "extends" reference so test builds drift from root strict compiler settings; update tsconfig.test.json to extend the central/base tsconfig (so it inherits strict flags like noUncheckedIndexedAccess, exactOptionalPropertyTypes, noImplicitOverride, noUnusedLocals, etc.), keep or override only test-specific compilerOptions (e.g., moduleResolution, target) and preserve "include": ["src","test"] and "exclude": ["node_modules","dist"] — ensure the "compilerOptions" in tsconfig.test.json only contains intentional overrides and that common strict flags come from the base config.packages/nuxt/test/middleware.test.ts (1)
241-256: Only the negative POST path is tested fordouble-submit; the happy path is absent.There is no test asserting that a POST carrying a valid token and matching cookie is accepted. Without it, a regression that rejects all POSTs would pass the suite silently.
✅ Suggested additional test
it('should accept POST with matching token for double-submit', async () => { const adapter = new NuxtAdapter() const protection = createCsrfProtection(adapter, { strategy: 'double-submit', }) // First issue a token via GET const getEvent = createGetEvent() await protection.protect(getEvent, getEvent) const issuedToken = getResponseHeaders()['x-csrf-token']! vi.clearAllMocks() // Submit it back in header + cookie const postEvent = createPostEvent( 'http://localhost/api', { 'x-csrf-token': issuedToken }, { 'csrf-token': issuedToken }, ) const result = await protection.protect(postEvent, postEvent) expect(result.success).toBe(true) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/test/middleware.test.ts` around lines 241 - 256, Add a positive-path unit test verifying the double-submit flow: use NuxtAdapter and createCsrfProtection(strategy: 'double-submit'), call protection.protect with a GET event (use createGetEvent) to obtain the issued token from getResponseHeaders()['x-csrf-token'], then clear mocks and submit a POST event (use createPostEvent) that sends the token both in the header ('x-csrf-token') and as a cookie ('csrf-token'), call protection.protect for the POST and assert result.success is true; reference NuxtAdapter, createCsrfProtection, createGetEvent, createPostEvent, getResponseHeaders, and protection.protect to locate where to add the test.packages/nuxt/src/runtime/plugin.client.ts (1)
12-16: DuplicatepublicConfigtype casting — consider a shared type.The same inline type
{ cookieName?: string; headerName?: string } | undefinedcast appears verbatim inpackages/nuxt/src/runtime/composables/useCsrfFetch.ts(lines 23–28). Defining this once (e.g.,CsrfArmorPublicConfig) prevents drift if the config shape grows.♻️ Proposed extraction
Create a shared
types.tsunderpackages/nuxt/src/runtime/:+// packages/nuxt/src/runtime/types.ts +export interface CsrfArmorPublicConfig { + cookieName?: string; + headerName?: string; +}Then in both files:
- const publicConfig = runtimeConfig.public.csrfArmor as - | { - cookieName?: string; - } - | undefined; + import type { CsrfArmorPublicConfig } from '../types'; + const publicConfig = runtimeConfig.public.csrfArmor as CsrfArmorPublicConfig | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/plugin.client.ts` around lines 12 - 16, The inline duplicate type for publicConfig (the `{ cookieName?: string; headerName?: string } | undefined` cast used when reading runtimeConfig.public.csrfArmor in publicConfig and in useCsrfFetch) should be extracted into a single exported type (e.g., CsrfArmorPublicConfig) in a new shared types file (create types.ts under the runtime folder), then import and use that type for the casts in both plugin.client.ts (where publicConfig is declared) and useCsrfFetch.ts to avoid drift and keep the shape centralized.packages/nuxt/package.json (1)
69-69:"module"field is redundant with theexportsmap.Modern bundlers and Node.js use the
exportsmap (lines 9–13) exclusively; the legacy"module"field at line 69 adds noise without benefit.♻️ Proposed cleanup
- "module": "./dist/module.js"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/package.json` at line 69, Remove the redundant "module" field from package.json: delete the "module": "./dist/module.js" property (it is superseded by the existing exports map) and ensure the resulting JSON stays valid (no stray trailing commas); confirm npm/ts/packaging builds still pass and that the "exports" map remains the single source of module entry points.packages/nuxt/src/runtime/server/middleware.ts (1)
12-31: A word of counsel on the singleton, if you will hear it.The
csrfProtectionsingleton is initialized once and never reset. In long-running dev servers with HMR, changes toruntimeConfig.csrfArmorwill not take effect until a full restart. This is acceptable for production but worth a comment for clarity.Also, Line 29:
config ?? undefinedis redundant —configis already typed asCsrfConfig | undefined, so the nullish coalescing serves no purpose.🧹 Minor cleanup
- csrfProtection = createCsrfProtection<H3Event, H3Event>( - adapter, - config ?? undefined - ); + csrfProtection = createCsrfProtection<H3Event, H3Event>(adapter, config);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/server/middleware.ts` around lines 12 - 31, The csrfProtection singleton is created once in the default event handler (csrfProtection, createCsrfProtection, NuxtAdapter, useRuntimeConfig and runtimeConfig.csrfArmor) which prevents runtimeConfig.csrfArmor changes from taking effect during long-running dev servers with HMR; add a clear comment above the initialization explaining this intentional singleton behaviour and that a full restart is required to pick up runtimeConfig changes in dev, and remove the redundant nullish coalescing (`config ?? undefined`) when passing config into createCsrfProtection since config is already typed as CsrfConfig | undefined.packages/nuxt/src/runtime/composables/useCsrfToken.ts (2)
11-15: Module-level singletons on the server — heed this counsel.
globalListenersInitializedandresolvedConfigare module-scoped. On the client this is fine (single app instance). On the server,initGlobalListenersis guarded byimport.meta.clientso it's a no-op — good. ButresolvedConfig(Line 15) is set on first invocation and cached indefinitely. Since it's derived fromruntimeConfig(which should be constant per deployment), this is acceptable, though it does mean runtime config changes require a process restart.A brief comment noting this intentional caching would be a kindness to future travelers on this road.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/composables/useCsrfToken.ts` around lines 11 - 15, The module currently uses module-scoped singletons globalListenersInitialized and resolvedConfig; add a concise comment near resolvedConfig explaining that the caching is intentional: resolvedConfig is derived from runtimeConfig and cached for the lifetime of the process (so changes to runtimeConfig require a process restart), and note that initGlobalListeners is guarded by import.meta.client so globalListenersInitialized is client-only — this documents the server-side singleton behavior and the intended trade-off.
41-48: The popstate listener — a watchful sentinel, yet with a curious delay.The 50ms
setTimeoutbefore refreshing the token onpopstateis a heuristic to let cookies settle after navigation. This is somewhat fragile — if the middleware's response hasn't been fully processed, the token read could still be stale. Consider documenting the rationale for the 50ms delay inline for maintainability, so that those who come after may understand why the wizard chose this particular number.Also, the
asynccallback withinsetTimeoutmeans any unhandled rejection insiderefreshCsrfTokenwould become an unhandled promise rejection. ThoughrefreshCsrfTokenwraps its logic in try/catch, if the catch itself threw, it would be unobserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/composables/useCsrfToken.ts` around lines 41 - 48, The popstate handler uses a 50ms setTimeout before calling refreshCsrfToken(config) which is a fragile heuristic and the async callback can surface unhandled rejections; add an inline comment next to the setTimeout explaining why 50ms was chosen (to allow cookies/middleware-driven Set-Cookie headers to settle after navigation) and when to revisit it, and change the callback to explicitly handle promise rejections (e.g. remove the async function passed to setTimeout and call refreshCsrfToken(config).then(newToken => { if (newToken && newToken !== csrfToken.value) csrfToken.value = newToken; }).catch(err => { /* log or ignore safely */ });) so that any errors from refreshCsrfToken are observed; reference the popstate listener, setTimeout(50), refreshCsrfToken and csrfToken.value in your update.packages/nuxt/src/runtime/server/adapter.ts (1)
127-142: Three paths that walk the same road — consider simplifying.All three content-type branches (form-urlencoded/multipart, JSON/LD+JSON, text/plain) call
readBody(event)identically. h3'sreadBodyalready handles content-type detection internally. The branching here serves only as documentation for which types are supported, while theelseclause (Line 140-141) setsparsedBody = nullfor unsupported types.You may wish to collapse this into a single allowlist check for clarity, though I understand the value of explicitness. A small refactor for the Fellowship's future maintainers:
♻️ Optional simplification
const contentType = getHeader(event, 'content-type') ?? 'text/plain'; try { - if ( - contentType.startsWith('application/x-www-form-urlencoded') || - contentType.startsWith('multipart/form-data') - ) { - parsedBody = await readBody(event); - } else if ( - contentType.startsWith('application/json') || - contentType.startsWith('application/ld+json') - ) { - parsedBody = await readBody(event); - } else if (contentType.startsWith('text/plain')) { + const supportedTypes = [ + 'application/x-www-form-urlencoded', + 'multipart/form-data', + 'application/json', + 'application/ld+json', + 'text/plain', + ]; + if (supportedTypes.some((t) => contentType.startsWith(t))) { parsedBody = await readBody(event); } else { parsedBody = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/server/adapter.ts` around lines 127 - 142, The code repeats identical calls to readBody(event) for multiple content-type branches (contentType checks) resulting in redundant paths; simplify by replacing the three identical branches with a single condition that calls readBody(event) when the contentType is in the supported allowlist (or simply when contentType is present and not explicitly unsupported) and otherwise sets parsedBody = null, keeping the variables contentType, parsedBody and the call to readBody(event) in adapter.ts so behavior is unchanged but the branching is collapsed for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/nuxt-app/nuxt.config.ts`:
- Around line 13-19: Replace the hardcoded CSRF secret in the csrfArmor config
(the secret property) with a runtime env var (use Nuxt runtime
config/NUXT_CSRF_SECRET) and update the code to fall back to a safe default only
for local dev; also add an inline comment next to cookie.secure (in the
csrfArmor.cookie block) stating it must be set to true in production and ensure
the code uses a runtime check (e.g., process.env.NODE_ENV !== 'production')
before allowing cookie.secure to be false.
In `@examples/nuxt-app/pages/demo/`[strategy].vue:
- Line 3: route.params.strategy is typed as string | string[] but is being
force-cast to string; replace the cast with a runtime-safe narrowing: read the
raw value from route.params.strategy, check Array.isArray(...) and if it is an
array pick a single element (e.g., first) or handle it as an error/redirect,
otherwise use the string value, and also guard for undefined/empty cases; update
the variable named strategy in the page (and any use-sites) to use this
narrowed/validated value instead of the direct cast.
- Around line 85-87: The template currently hardcodes "CSRF validation failed"
inside the div bound to v-if="error" (the <div v-if="error" class="error">
block), which mislabels non-CSRF failures; update that div to display a generic,
dynamic message instead (e.g., "Request failed:" plus the actual error details)
by rendering the error payload (error.message || String(error) or
JSON.stringify(error)) and optionally show a CSRF-specific message only when the
error clearly indicates a CSRF token problem (e.g., check error.code ===
'EBADCSRFTOKEN' or error.message includes 'CSRF') so other failures (network,
500, parse errors) are labeled correctly.
In `@packages/nuxt/package.json`:
- Around line 36-48: The repository, bugs and homepage fields in package.json
currently point to the PR author's fork; update the "repository.url",
"bugs.url", and "homepage" values to the canonical upstream URL
"https://github.com/muneebs/csrf-armor" (keep the "directory": "packages/nuxt"
entry unchanged) so published npm metadata links to the correct source, issue
tracker and README.
In `@packages/nuxt/src/runtime/composables/useCsrfFetch.ts`:
- Around line 45-51: The current guard in useCsrfFetch.ts that only invokes
opts.onRequest when it's a function will drop handlers if opts.onRequest is an
array; update the invocation logic in the onRequest block (where opts.onRequest
and context are referenced) to accept both a single FetchHook function and an
array of FetchHook functions: if opts.onRequest is a function call it with
context, and if it's an array iterate over the array and call each handler
(awaiting each if they return a Promise) so all provided interceptors run rather
than silently being discarded.
In `@packages/nuxt/src/runtime/utils/client.ts`:
- Around line 26-32: The cookie parsing in get csrf code uses
csrfCookie.split('=') which splits on every '=' and truncates base64-padding;
update the extraction logic (in the block that finds csrfCookie using cookieName
and csrfCookie variable) to locate the first '=' (e.g., via indexOf) and slice
the substring after that first '=' so the full value (including any '=' padding)
is captured, then decodeURIComponent(trimmedValue) and return it; ensure you
reference the csrfCookie and cookieName variables and avoid splitting on all '='
characters.
---
Nitpick comments:
In @.editorconfig:
- Around line 14-16: The .editorconfig section for [{pnpm-lock.yaml,*.lock}]
currently unsets indent_size and insert_final_newline but leaves
trim_trailing_whitespace inherited (true); update that section by adding
trim_trailing_whitespace = unset so the lock-file pattern in .editorconfig (the
[{pnpm-lock.yaml,*.lock}] section) explicitly disables trimming trailing
whitespace.
In `@packages/nuxt/package.json`:
- Line 69: Remove the redundant "module" field from package.json: delete the
"module": "./dist/module.js" property (it is superseded by the existing exports
map) and ensure the resulting JSON stays valid (no stray trailing commas);
confirm npm/ts/packaging builds still pass and that the "exports" map remains
the single source of module entry points.
In `@packages/nuxt/src/runtime/composables/useCsrfToken.ts`:
- Around line 11-15: The module currently uses module-scoped singletons
globalListenersInitialized and resolvedConfig; add a concise comment near
resolvedConfig explaining that the caching is intentional: resolvedConfig is
derived from runtimeConfig and cached for the lifetime of the process (so
changes to runtimeConfig require a process restart), and note that
initGlobalListeners is guarded by import.meta.client so
globalListenersInitialized is client-only — this documents the server-side
singleton behavior and the intended trade-off.
- Around line 41-48: The popstate handler uses a 50ms setTimeout before calling
refreshCsrfToken(config) which is a fragile heuristic and the async callback can
surface unhandled rejections; add an inline comment next to the setTimeout
explaining why 50ms was chosen (to allow cookies/middleware-driven Set-Cookie
headers to settle after navigation) and when to revisit it, and change the
callback to explicitly handle promise rejections (e.g. remove the async function
passed to setTimeout and call refreshCsrfToken(config).then(newToken => { if
(newToken && newToken !== csrfToken.value) csrfToken.value = newToken;
}).catch(err => { /* log or ignore safely */ });) so that any errors from
refreshCsrfToken are observed; reference the popstate listener, setTimeout(50),
refreshCsrfToken and csrfToken.value in your update.
In `@packages/nuxt/src/runtime/plugin.client.ts`:
- Around line 12-16: The inline duplicate type for publicConfig (the `{
cookieName?: string; headerName?: string } | undefined` cast used when reading
runtimeConfig.public.csrfArmor in publicConfig and in useCsrfFetch) should be
extracted into a single exported type (e.g., CsrfArmorPublicConfig) in a new
shared types file (create types.ts under the runtime folder), then import and
use that type for the casts in both plugin.client.ts (where publicConfig is
declared) and useCsrfFetch.ts to avoid drift and keep the shape centralized.
In `@packages/nuxt/src/runtime/server/adapter.ts`:
- Around line 127-142: The code repeats identical calls to readBody(event) for
multiple content-type branches (contentType checks) resulting in redundant
paths; simplify by replacing the three identical branches with a single
condition that calls readBody(event) when the contentType is in the supported
allowlist (or simply when contentType is present and not explicitly unsupported)
and otherwise sets parsedBody = null, keeping the variables contentType,
parsedBody and the call to readBody(event) in adapter.ts so behavior is
unchanged but the branching is collapsed for clarity.
In `@packages/nuxt/src/runtime/server/middleware.ts`:
- Around line 12-31: The csrfProtection singleton is created once in the default
event handler (csrfProtection, createCsrfProtection, NuxtAdapter,
useRuntimeConfig and runtimeConfig.csrfArmor) which prevents
runtimeConfig.csrfArmor changes from taking effect during long-running dev
servers with HMR; add a clear comment above the initialization explaining this
intentional singleton behaviour and that a full restart is required to pick up
runtimeConfig changes in dev, and remove the redundant nullish coalescing
(`config ?? undefined`) when passing config into createCsrfProtection since
config is already typed as CsrfConfig | undefined.
In `@packages/nuxt/test/middleware.test.ts`:
- Around line 241-256: Add a positive-path unit test verifying the double-submit
flow: use NuxtAdapter and createCsrfProtection(strategy: 'double-submit'), call
protection.protect with a GET event (use createGetEvent) to obtain the issued
token from getResponseHeaders()['x-csrf-token'], then clear mocks and submit a
POST event (use createPostEvent) that sends the token both in the header
('x-csrf-token') and as a cookie ('csrf-token'), call protection.protect for the
POST and assert result.success is true; reference NuxtAdapter,
createCsrfProtection, createGetEvent, createPostEvent, getResponseHeaders, and
protection.protect to locate where to add the test.
In `@packages/nuxt/tsconfig.test.json`:
- Around line 1-13: The test tsconfig (tsconfig.test.json) is missing an
"extends" reference so test builds drift from root strict compiler settings;
update tsconfig.test.json to extend the central/base tsconfig (so it inherits
strict flags like noUncheckedIndexedAccess, exactOptionalPropertyTypes,
noImplicitOverride, noUnusedLocals, etc.), keep or override only test-specific
compilerOptions (e.g., moduleResolution, target) and preserve "include":
["src","test"] and "exclude": ["node_modules","dist"] — ensure the
"compilerOptions" in tsconfig.test.json only contains intentional overrides and
that common strict flags come from the base config.
In `@packages/nuxt/vitest.config.ts`:
- Around line 4-21: The tsconfigRaw object in defineConfig duplicates
compilerOptions already present in tsconfig.test.json; replace the inline
JSON.stringify block with an imported object reference by reading/parsing
tsconfig.test.json and assigning its compilerOptions (or the whole config)
directly to tsconfigRaw (no JSON.stringify needed). Locate the defineConfig call
and the tsconfigRaw property in vitest.config.ts, import or require the
tsconfig.test.json, and set tsconfigRaw: importedConfig (or
tsconfig.test.json.compilerOptions) to remove the duplicate source of truth.
| csrfArmor: { | ||
| strategy: 'signed-double-submit', | ||
| secret: 'super-secret-key-for-dev-only-32-chars-long-enough', | ||
| token: { expiry: 3600, fieldName: '_csrf' }, | ||
| cookie: { secure: false, name: 'x-csrf-token' }, | ||
| allowedOrigins: ['http://localhost:3000'], | ||
| }, |
There was a problem hiding this comment.
Do not commit a hardcoded CSRF secret — even in examples.
A secret with its value committed to a public repository is no secret at all. Even though "dev-only" appears in the string, developers routinely copy example configs into production without reading the embedded warning. Replace it with an environment variable — Nuxt's runtime config convention (NUXT_ prefix) is already the right tool here. While you are at it, add an inline note that cookie.secure must be true in production.
🛡️ Proposed fix
csrfArmor: {
strategy: 'signed-double-submit',
- secret: 'super-secret-key-for-dev-only-32-chars-long-enough',
+ secret: process.env.NUXT_CSRF_ARMOR_SECRET ?? 'dev-only-fallback-replace-in-production',
token: { expiry: 3600, fieldName: '_csrf' },
- cookie: { secure: false, name: 'x-csrf-token' },
+ cookie: { secure: process.env.NODE_ENV === 'production', name: 'x-csrf-token' },
allowedOrigins: ['http://localhost:3000'],
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/nuxt-app/nuxt.config.ts` around lines 13 - 19, Replace the hardcoded
CSRF secret in the csrfArmor config (the secret property) with a runtime env var
(use Nuxt runtime config/NUXT_CSRF_SECRET) and update the code to fall back to a
safe default only for local dev; also add an inline comment next to
cookie.secure (in the csrfArmor.cookie block) stating it must be set to true in
production and ensure the code uses a runtime check (e.g., process.env.NODE_ENV
!== 'production') before allowing cookie.secure to be false.
- fix critical base64 cookie parsing truncation in both nuxt and nextjs client utils (split on first '=' only via indexOf + slice) - fix silent loss of onRequest array handlers in useCsrfFetch - fix unsafe route param cast with runtime Array.isArray narrowing - fix misleading "CSRF validation failed" error label in demo page - fix repo URLs pointing to fork instead of upstream - simplify adapter readBody branches into supportedTypes allowlist - extract shared CsrfArmorPublicConfig type to prevent drift - replace typeof window checks with idiomatic import.meta.client - remove redundant nullish coalescing in middleware singleton init - document singleton caching and popstate 50ms delay rationale - handle unobserved promise rejections in popstate listener - add happy-path double-submit POST acceptance test - add trim_trailing_whitespace = unset for lock files in editorconfig
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/nextjs/src/client/client.ts (1)
63-65: The fix is correct — a wizard never truncates base64 padding!Using
indexOf+sliceinstead of the oldsplit('=')[1]properly preserves base64 padding (==) and any=characters embedded in the token value. The guard oneqIndex !== -1, however, is a dead branch:csrfCookiewas already matched by.startsWith(${cookieName}=), soindexOf('=')is always> -1.♻️ Simplify the dead-branch ternary
- const eqIndex = csrfCookie.indexOf('='); - const value = eqIndex !== -1 ? csrfCookie.slice(eqIndex + 1) : ''; - return decodeURIComponent(value.trim()); + return decodeURIComponent(csrfCookie.slice(csrfCookie.indexOf('=') + 1).trim());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/client/client.ts` around lines 63 - 65, The ternary guarding against eqIndex === -1 is dead because csrfCookie is already validated with .startsWith(`${cookieName}=`); simplify by removing the conditional and unconditionally compute the value from the first '=' onward (use csrfCookie.indexOf('=') to locate it or directly use csrfCookie.slice(cookieName.length + 1)); update the lines around csrfCookie, eqIndex, and value in client.ts to return decodeURIComponent of the trimmed sliced value without the unused branch.packages/nuxt/src/runtime/composables/useCsrfToken.ts (2)
79-84: There is no need to forge a new ring when one already exists.This inline type exactly duplicates
CsrfArmorPublicConfigdefined in./types.ts. Import and use it instead.♻️ Proposed fix
+import type { CsrfArmorPublicConfig } from '../types'; // ... - const publicConfig = runtimeConfig.public.csrfArmor as - | { - cookieName?: string; - headerName?: string; - } - | undefined; + const publicConfig = runtimeConfig.public.csrfArmor as + | CsrfArmorPublicConfig + | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/composables/useCsrfToken.ts` around lines 79 - 84, Replace the duplicated inline type for publicConfig in useCsrfToken.ts with the existing CsrfArmorPublicConfig type from ./types: import CsrfArmorPublicConfig and change the declared type of publicConfig to runtimeConfig.public.csrfArmor as CsrfArmorPublicConfig | undefined; update any references to the inline shape accordingly and remove the inline type block so the code uses the shared CsrfArmorPublicConfig definition.
118-118: Speak plainly; do not say twice what need be said once.
config.headerNameis always a non-null string —resolvedConfigunconditionally assigns it at line 89 (?? 'x-csrf-token'). The?? 'x-csrf-token'on line 118 is unreachable dead code.♻️ Proposed simplification
- const headerName = config.headerName ?? 'x-csrf-token'; + const headerName = config.headerName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/composables/useCsrfToken.ts` at line 118, The nullish-coalescing fallback on the local binding is dead code because resolvedConfig already guarantees a non-null headerName; replace "const headerName = config.headerName ?? 'x-csrf-token';" with a direct read "const headerName = config.headerName;" (or use the already resolved value from resolvedConfig/headerName) to remove the redundant '??' fallback in useCsrfToken.ts.packages/nuxt/src/runtime/server/adapter.ts (1)
176-187: Do not carry unnecessary burdens on your journey.
adaptCookieOptionsspreads everyCookieOptionsfield unconditionally, including those that areundefinedwhen not set. Whileh3/cookie-essilently ignoresundefinedproperties during serialization, the explicitundefinedkeys pollute the returned object and weaken its type contract. Filtering them out is the cleaner approach:♻️ Proposed fix
- private adaptCookieOptions(options?: CookieOptions): Record<string, unknown> { + private adaptCookieOptions(options?: CookieOptions): Record<string, unknown> { if (!options) return {}; - return { - secure: options.secure, - httpOnly: options.httpOnly, - sameSite: options.sameSite, - path: options.path, - domain: options.domain, - maxAge: options.maxAge, - }; + const raw: Record<string, unknown> = { + secure: options.secure, + httpOnly: options.httpOnly, + sameSite: options.sameSite, + path: options.path, + domain: options.domain, + maxAge: options.maxAge, + }; + return Object.fromEntries(Object.entries(raw).filter(([, v]) => v !== undefined)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/server/adapter.ts` around lines 176 - 187, The adaptCookieOptions function currently returns all CookieOptions fields even when undefined; update adaptCookieOptions to omit keys with undefined values so the returned Record<string, unknown> only contains defined properties (secure, httpOnly, sameSite, path, domain, maxAge) — e.g., build the object and filter out undefined entries (using a reduce/Object.entries filter or conditional assignments) before returning; keep the function name adaptCookieOptions and its CookieOptions input shape intact and ensure behavior for a missing options argument still returns {}.packages/nuxt/src/runtime/plugin.client.ts (1)
22-23: Even a wizard need not cast the same spell twice.The factory
() => tokenin theuseStatecall (line 22) will never actually run on the client because the state is already hydrated from SSR; the real update is line 23. The factory is therefore dead code here. Simplifying removes the ambiguity:♻️ Proposed simplification
- // Shared with useCsrfToken() composable via the same useState key - const csrfToken = useState<string | null>('csrf-token', () => token); - csrfToken.value = token; + // Shared with useCsrfToken() composable via the same useState key. + // Factory is intentionally omitted: state is SSR-hydrated (null) and we + // always overwrite with the live cookie value on client mount. + const csrfToken = useState<string | null>('csrf-token'); + csrfToken.value = token;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/plugin.client.ts` around lines 22 - 23, The factory passed to useState is dead code because SSR hydration provides the value and you then overwrite it on the next line; replace the factory that returns token with a simple empty initial value and keep the explicit assignment. Specifically, update the useState call for the 'csrf-token' state (csrfToken and useState) to use a plain null/empty initial value (e.g., useState<string | null>('csrf-token', null) or useState<string | null>('csrf-token', () => null)) and leave the existing csrfToken.value = token assignment as-is.packages/nuxt/test/middleware.test.ts (1)
6-15: Even the smallest gap in armour may let an arrow through.
readBodyis included in thevi.mockfactory but is never extracted as a typed mock variable (unlikemockedGetHeader,mockedParseCookies, etc.). This means no test currently exercises the body-based token extraction path (getTokenFromRequestpriority#3in the adapter). Consider addingmockedReadBodyand a test that submits the CSRF token exclusively in the JSON body.♻️ Suggested extraction and skeleton test
+const mockedReadBody = vi.mocked(readBody)Skeleton for the missing path:
+ it('should extract token from JSON body for double-submit', async () => { + const adapter = new NuxtAdapter() + const protection = createCsrfProtection(adapter, { strategy: 'double-submit' }) + + const getEvent = createGetEvent() + await protection.protect(getEvent, getEvent) + const issuedToken = getResponseHeaders()['x-csrf-token']! + vi.clearAllMocks() + + mockedReadBody.mockResolvedValue({ 'csrf-token': issuedToken }) + const postEvent = createPostEvent( + 'http://localhost/api', + { 'content-type': 'application/json' }, + { 'csrf-token': issuedToken }, + ) + const result = await protection.protect(postEvent, postEvent) + expect(result.success).toBe(true) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/test/middleware.test.ts` around lines 6 - 15, The mock factory exposes h3.readBody but the test file never extracts it as a typed mock; add a typed mock variable (e.g. mockedReadBody = vi.mocked(readBody)) alongside mockedGetHeader/mockedParseCookies and use it in a new test that returns a JSON body containing only the CSRF token to exercise the body-based extraction path (getTokenFromRequest / adapter priority `#3`). In the new test, set mockedReadBody to resolve the token payload, call the middleware/token extraction helper under test, assert the returned token matches the body value, and remember to reset/clear mocks between tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nextjs/src/client/client.ts`:
- Line 65: Wrap the decodeURIComponent(value.trim()) call in a defensive
try/catch inside the function that reads the cookie (the spot that currently
returns decodeURIComponent(value.trim())), catching URIError (or any thrown
error) and returning null when decoding fails; this prevents malformed
percent-encoding in cookie values from throwing and lets callers receive null
instead of crashing.
In `@packages/nuxt/test/middleware.test.ts`:
- Line 164: Replace the raw test secret literal with an explicit inline
secret-scan suppression on the declaration of the const secret in
middleware.test.ts (the variable named "secret") so scanners ignore this known
test fixture; add the project's gitleaks/secret-scan directive (for example use
the repository's approved inline rule such as a gitleaks allow comment) on the
same line and include a short rationale like "test fixture" to make intent
explicit for reviewers and CI.
---
Nitpick comments:
In `@packages/nextjs/src/client/client.ts`:
- Around line 63-65: The ternary guarding against eqIndex === -1 is dead because
csrfCookie is already validated with .startsWith(`${cookieName}=`); simplify by
removing the conditional and unconditionally compute the value from the first
'=' onward (use csrfCookie.indexOf('=') to locate it or directly use
csrfCookie.slice(cookieName.length + 1)); update the lines around csrfCookie,
eqIndex, and value in client.ts to return decodeURIComponent of the trimmed
sliced value without the unused branch.
In `@packages/nuxt/src/runtime/composables/useCsrfToken.ts`:
- Around line 79-84: Replace the duplicated inline type for publicConfig in
useCsrfToken.ts with the existing CsrfArmorPublicConfig type from ./types:
import CsrfArmorPublicConfig and change the declared type of publicConfig to
runtimeConfig.public.csrfArmor as CsrfArmorPublicConfig | undefined; update any
references to the inline shape accordingly and remove the inline type block so
the code uses the shared CsrfArmorPublicConfig definition.
- Line 118: The nullish-coalescing fallback on the local binding is dead code
because resolvedConfig already guarantees a non-null headerName; replace "const
headerName = config.headerName ?? 'x-csrf-token';" with a direct read "const
headerName = config.headerName;" (or use the already resolved value from
resolvedConfig/headerName) to remove the redundant '??' fallback in
useCsrfToken.ts.
In `@packages/nuxt/src/runtime/plugin.client.ts`:
- Around line 22-23: The factory passed to useState is dead code because SSR
hydration provides the value and you then overwrite it on the next line; replace
the factory that returns token with a simple empty initial value and keep the
explicit assignment. Specifically, update the useState call for the 'csrf-token'
state (csrfToken and useState) to use a plain null/empty initial value (e.g.,
useState<string | null>('csrf-token', null) or useState<string |
null>('csrf-token', () => null)) and leave the existing csrfToken.value = token
assignment as-is.
In `@packages/nuxt/src/runtime/server/adapter.ts`:
- Around line 176-187: The adaptCookieOptions function currently returns all
CookieOptions fields even when undefined; update adaptCookieOptions to omit keys
with undefined values so the returned Record<string, unknown> only contains
defined properties (secure, httpOnly, sameSite, path, domain, maxAge) — e.g.,
build the object and filter out undefined entries (using a reduce/Object.entries
filter or conditional assignments) before returning; keep the function name
adaptCookieOptions and its CookieOptions input shape intact and ensure behavior
for a missing options argument still returns {}.
In `@packages/nuxt/test/middleware.test.ts`:
- Around line 6-15: The mock factory exposes h3.readBody but the test file never
extracts it as a typed mock; add a typed mock variable (e.g. mockedReadBody =
vi.mocked(readBody)) alongside mockedGetHeader/mockedParseCookies and use it in
a new test that returns a JSON body containing only the CSRF token to exercise
the body-based extraction path (getTokenFromRequest / adapter priority `#3`). In
the new test, set mockedReadBody to resolve the token payload, call the
middleware/token extraction helper under test, assert the returned token matches
the body value, and remember to reset/clear mocks between tests.
packages/nextjs/src/client/client.ts
Outdated
| return decodeURIComponent(value?.trim() ?? ''); | ||
| const eqIndex = csrfCookie.indexOf('='); | ||
| const value = eqIndex !== -1 ? csrfCookie.slice(eqIndex + 1) : ''; | ||
| return decodeURIComponent(value.trim()); |
There was a problem hiding this comment.
Guard decodeURIComponent against malformed sequences, lest the cookie values lead you into shadow.
A stray % or invalid percent-encoded sequence in the cookie value will cause decodeURIComponent to throw a URIError, crashing callers instead of gracefully returning null.
🛡️ Proposed defensive fix
- return decodeURIComponent(value.trim());
+ try {
+ return decodeURIComponent(value.trim());
+ } catch {
+ return value.trim();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return decodeURIComponent(value.trim()); | |
| try { | |
| return decodeURIComponent(value.trim()); | |
| } catch { | |
| return value.trim(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nextjs/src/client/client.ts` at line 65, Wrap the
decodeURIComponent(value.trim()) call in a defensive try/catch inside the
function that reads the cookie (the spot that currently returns
decodeURIComponent(value.trim())), catching URIError (or any thrown error) and
returning null when decoding fails; this prevents malformed percent-encoding in
cookie values from throwing and lets callers receive null instead of crashing.
The csrfArmor config is mapped to runtimeConfig by the module, making all keys overridable via NUXT_ prefixed environment variables at runtime.
- Simplify cookie value parsing (remove dead ternary, add try/catch for decodeURIComponent on malformed values) in both nuxt and nextjs - Use shared CsrfArmorPublicConfig type in useCsrfToken composable - Remove dead ?? fallback for headerName (already resolved in config) - Add gitleaks:allow annotation on test fixture secret - Add body-extraction test for double-submit POST with JSON body - Fix TS2742 DTS portability error in nuxt module export
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/nuxt/src/runtime/composables/useCsrfToken.ts (2)
16-23: Module-level mutable state shared across SSR requests — safe here, but tread carefully.
resolvedConfigandglobalListenersInitializedare module-scoped singletons. In Nuxt SSR, the module is shared across concurrent requests on the server. This is safe because:
globalListenersInitializedis guarded byimport.meta.client(line 34), so it's only mutated on the client.resolvedConfigis derived fromruntimeConfig.public, which is identical across requests.A brief inline comment noting why this is safe would spare future travellers from doubt, for not all who wander through module-level state find their way so easily.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/composables/useCsrfToken.ts` around lines 16 - 23, Add a brief inline comment above the module-level variables explaining why module-level mutable state is safe here: note that globalListenersInitialized is only mutated on the client (guarded by import.meta.client) and resolvedConfig is derived from runtimeConfig.public which is identical across SSR requests, so concurrent server requests won't cause cross-request state issues; reference the symbols resolvedConfig and globalListenersInitialized and mention the runtimeConfig.public origin to clarify for future readers.
39-63: The route watcher and the popstate listener both seek the same prize — consider whether both quests are needed.On browser back/forward navigation, both the
watch(route.path)handler (line 41) and thepopstatelistener (line 51) will fire. The route watcher synchronously reads the cookie, and ~50ms later the popstate handler issues a HEAD request viarefreshCsrfTokenand potentially overwrites the token again.Two observations:
Redundant update path: The route watcher's synchronous cookie read will be superseded by the popstate handler's async refresh if the token differs. This is benign but may cause a brief flicker in the reactive state.
The 50ms delay is a magic number (line 62). On slow connections or heavy server load, the middleware
Set-Cookieheader may not have settled in 50ms. Consider either extracting this to a named constant with a comment explaining its purpose, or using the route watcher alone (since client-side navigations through Nuxt's router trigger the server middleware, which sets the cookie before the response arrives).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/composables/useCsrfToken.ts` around lines 39 - 63, The popstate handler duplicates the route watcher: both watch(() => route.path) using getCsrfToken and the window.addEventListener('popstate') using refreshCsrfToken race and cause a transient overwrite (and the 50ms is a magic number). Remove the popstate listener block (the setTimeout + refreshCsrfToken call) and rely on the existing watch(route.path) + getCsrfToken flow to update csrfToken.value; if you must keep an async refresh path instead extract the 50 (ms) into a named constant (e.g. CSRF_REFRESH_DELAY_MS) with a comment and ensure refreshCsrfToken is only invoked when getCsrfToken would produce a different value to avoid redundant updates.packages/nuxt/src/runtime/utils/client.ts (1)
1-116: The core logic echoes between realms, yet masquerades in different vestments — consider a shared foundation.Thou speakest true of duplication, yet the truth is more nuanced than "line-for-line identical." The Nuxt adaptation is nearly half the length of its Next.js counterpart: whilst the Next.js version bears extensive JSDoc with examples and explanatory comments, the Nuxt version stands trimmed to its essence. Yet beneath these surface differences, the logic rings identical.
The functions
getCsrfToken,createCsrfHeaders,csrfFetch, andrefreshCsrfTokenall follow the same paths, differing only in their environment guards (import.meta.clientvs.typeof window === 'undefined'). Should one copy receive a bugfix and the other does not, inconsistency shall creep in like shadow at eventide.Consider extracting the shared logic—cookie parsing, header construction, token management—into
@csrf-armor/core, parameterizing only the environment check. Each framework would then wield thin shims atop a single source of truth. This need not impede thy current journey, but I counsel thee: address it ere another adapter joins the fellowship, lest the duplication multiply beyond redemption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxt/src/runtime/utils/client.ts` around lines 1 - 116, The four functions getCsrfToken, createCsrfHeaders, csrfFetch and refreshCsrfToken duplicate logic with only the environment guard differing (import.meta.client); extract the shared cookie parsing, header construction and token refresh logic into a single core module (e.g., `@csrf-armor/core`) that exposes pure helpers (parseCookieToken, buildCsrfHeaders, fetchWithCsrf, refreshToken) and have this file become a thin shim that only performs the environment check (import.meta.client) and delegates to those core helpers, passing in config and any env-specific behavior (like fetch/headers access); update this file to call the core helpers rather than reimplement logic so future fixes are applied once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/nuxt/src/runtime/utils/client.ts`:
- Around line 21-41: The getCsrfToken implementation is correct and requires no
change: keep the cookie parsing that uses slice(csrfCookie.indexOf('=') + 1) to
preserve base64 padding and the try/catch around decodeURIComponent in function
getCsrfToken; instead, add or ensure unit/integration tests cover a cookie value
with '=' padding and the meta tag fallback (meta[name="csrf-token"]) to prevent
regressions.
---
Nitpick comments:
In `@packages/nuxt/src/runtime/composables/useCsrfToken.ts`:
- Around line 16-23: Add a brief inline comment above the module-level variables
explaining why module-level mutable state is safe here: note that
globalListenersInitialized is only mutated on the client (guarded by
import.meta.client) and resolvedConfig is derived from runtimeConfig.public
which is identical across SSR requests, so concurrent server requests won't
cause cross-request state issues; reference the symbols resolvedConfig and
globalListenersInitialized and mention the runtimeConfig.public origin to
clarify for future readers.
- Around line 39-63: The popstate handler duplicates the route watcher: both
watch(() => route.path) using getCsrfToken and the
window.addEventListener('popstate') using refreshCsrfToken race and cause a
transient overwrite (and the 50ms is a magic number). Remove the popstate
listener block (the setTimeout + refreshCsrfToken call) and rely on the existing
watch(route.path) + getCsrfToken flow to update csrfToken.value; if you must
keep an async refresh path instead extract the 50 (ms) into a named constant
(e.g. CSRF_REFRESH_DELAY_MS) with a comment and ensure refreshCsrfToken is only
invoked when getCsrfToken would produce a different value to avoid redundant
updates.
In `@packages/nuxt/src/runtime/utils/client.ts`:
- Around line 1-116: The four functions getCsrfToken, createCsrfHeaders,
csrfFetch and refreshCsrfToken duplicate logic with only the environment guard
differing (import.meta.client); extract the shared cookie parsing, header
construction and token refresh logic into a single core module (e.g.,
`@csrf-armor/core`) that exposes pure helpers (parseCookieToken, buildCsrfHeaders,
fetchWithCsrf, refreshToken) and have this file become a thin shim that only
performs the environment check (import.meta.client) and delegates to those core
helpers, passing in config and any env-specific behavior (like fetch/headers
access); update this file to call the core helpers rather than reimplement logic
so future fixes are applied once.
|
Hi @Jorgagu thank you for the PR. Can you check the failing tests please. |
Closes #38
Summary by CodeRabbit
New Features
Chores
Tests